Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

wallet: fix makeBatch to generate addresses early #825

Merged
merged 1 commit into from
Jul 14, 2023

Conversation

rithvikvibhu
Copy link
Member

Bid actions in makeBatch had their addresses overridden after nonce generation, making it impossible to recover the wallet with importnonce.

With this change, the address is derived and passed to make* so the correct address is used to generate nonces.

The test covers both sendbid and sendbatch.

* @returns {Promise<MTX>}
*/

async makeOpen(name, acct, mtx) {
async makeOpen(name, acct, mtx, addr) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If addr is provided then acct has no use.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm true. But acct is mandatory and also matches other make* methods signatures. Not sure what can be done

@coveralls
Copy link

coveralls commented Jul 13, 2023

Coverage Status

coverage: 68.541% (-0.007%) from 68.548% when pulling c4aeec4 on rithvikvibhu:makebatch-addr-fix into 90cdf84 on handshake-org:master.

lib/wallet/wallet.js Outdated Show resolved Hide resolved
@rithvikvibhu rithvikvibhu force-pushed the makebatch-addr-fix branch 3 times, most recently from cd94b88 to 5ed8d30 Compare July 14, 2023 14:51
@nodech
Copy link
Contributor

nodech commented Jul 14, 2023

So, previous code was generating new address for every bid, but because receiveAddress in makeBid does not increment the depth it was using the cleanup phase to repopulate them with different depth/addresses. Also for the BID it's critical for constructing nonce and now we can pass these addrs even before, so we can avoid this issue.

Do we really want to pass ADDR to the OPEN though ? I believe it does not use that address for anything and can easily be outside. Only BID has trouble with it.

@nodech nodech added wallet part of the codebase patch Release version labels Jul 14, 2023
@rithvikvibhu
Copy link
Member Author

rithvikvibhu commented Jul 14, 2023

Okay now only bids get new addresses.
OPEN is unchanged, still gets new addresses in cleanup.

Bid actions in makeBatch had their addresses overridden after nonce generation,
making it impossible to recover the wallet with importnonce.
With this change, the address is derived and passed to make*
so the correct address is used to generate nonces.
@nodech nodech merged commit 81bddcd into handshake-org:master Jul 14, 2023
@rithvikvibhu rithvikvibhu deleted the makebatch-addr-fix branch July 15, 2023 03:53
nodech added a commit that referenced this pull request Jul 17, 2023
@nodech nodech mentioned this pull request Jul 17, 2023
nodech added a commit that referenced this pull request Jul 17, 2023
This was referenced Aug 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
patch Release version wallet part of the codebase
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants